Feature/update cloud example#345
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the “Cloud Environment with Re-imaging” example notebook to reflect the updated observation/action space and demonstrates new greedy heuristics plus a custom token/attention-based actor-critic architecture for RLlib PPO. Also documents the update in the v1.3.0 release notes.
Changes:
- Refresh the environment configuration (observations/actions/lookahead) and reward logic in
cloud_environment_with_reimaging.ipynb. - Add greedy heuristic baselines (re-imaging and single-picture) and a custom RLlib
RLModulewith attention-based actor/critic definitions. - Add a release note entry indicating the example has been updated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| examples/cloud_environment_with_reimaging.ipynb | Updates the cloud re-imaging tutorial with new observation/action definitions, heuristics, and custom actor-critic/RLModule code. |
| docs/source/release_notes.rst | Adds a release note entry referencing the updated example notebook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
552e14c to
ddd4323
Compare
ddd4323 to
0fcd701
Compare
0fcd701 to
a41a458
Compare
| " dict(prop=\"v_BN_P\", norm=7616.5),\n", | ||
| " dict(prop=\"battery_charge_fraction\"),\n", | ||
| " dict(prop=\"solar_angle_norm\"),\n", | ||
| " dict(prop=\"omega_BN_B\", norm=0.03), # 0,1,2 - CHECKED\n", |
There was a problem hiding this comment.
Those "CHECKED" comments should be removed.
| " log_level=\"INFO\",\n", | ||
| " log_level=\"WARNING\",\n", | ||
| " failure_penalty=0.0,\n", | ||
| " # disable_env_checker=True, # For debugging\n", |
There was a problem hiding this comment.
I think the "disable_env_checker" doesn't exist anymore and we need to remove this.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## Training and custom actor-critic modules\n", |
There was a problem hiding this comment.
I feel like this code is too much for one example script. Have you thought about splitting the code into two? For cloud with re-imaging example. And training with new architecture script?
| "* `belief` represents the probability that the target was successfully observed.\n", | ||
| "\n", | ||
| "* `prev_obs` time at which the last picture of the target was taken." | ||
| ] |
There was a problem hiding this comment.
Add explanation for "belief_update_var"
| "R = \\begin{cases}\n", | ||
| "\\rho_i\\alpha_i\\Delta \\text{P}(S=1) + \\rho_i(1 - \\alpha) & \\text{ if } \\text{P}_i(S=1) \\geq \\theta_{\\text{thr}_i} \\\\\n", | ||
| "\\rho_i\\alpha_i\\Delta \\text{P}(S=1) & \\text{ otherwise.}\n", | ||
| "r_i\\alpha_i\\Delta \\text{P}(S=1) + r_i(1 - \\alpha) & \\text{ if } \\text{P}_i(S=1) \\geq \\theta_{\\text{thr}_i} \\\\\n", |
There was a problem hiding this comment.
The alpha and alpha_i the same, right? If so, make it consistent. If not, explain the difference.
| " self.reward_fn = reward_fn\n", | ||
| " self.alpha = alpha\n", | ||
| "\n", | ||
| " def initial_data(self, satellite: \"sats.Satellite\") -> \"UniqueImageData\":\n", |
There was a problem hiding this comment.
Doesn't this have to be "CloudImageProbabilityData" and not "UniqueImageData"?
| " float: Reward for the target.\n", | ||
| " \"\"\"\n", | ||
| " if reach_threshold:\n", | ||
| " return priority * (1 - alpha)\n", |
There was a problem hiding this comment.
Isn't this "+ priority * belief_variation * alpha"? I thought the variation is also applied even when it reaches the threshold.
| "outputs": [], | ||
| "source": [ | ||
| "def time_variation(\n", | ||
| " delta_t: float, t_const: float, k_1: float = 2.5, k_2: float = 2.5, k_3: float = 1.0\n", |
There was a problem hiding this comment.
Use consistent parameters as in the equation (e or eta instead of k).
| " Returns:\n", | ||
| " np.array: Updated belief array\n", | ||
| " \"\"\"\n", | ||
| " time_constant = 30 * 60 / 5 # 30 minutes\n", |
There was a problem hiding this comment.
Is it 30 mins to get the sigmoid function to be 1? It's not 30 mins just for the equation.
| " return r_SB_H / np.linalg.norm(r_SB_H)\n", | ||
| "\n", | ||
| "\n", | ||
| "class CloudCoverDensity(obs.Observation):\n", |
There was a problem hiding this comment.
Add this class explanation somewhere.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## Configuring the Satellite to Have Access to Cloud Information\n", |
There was a problem hiding this comment.
It would be great if you could briefly explain what each satellite is (difference between CS_F and CS_F_Bayesian).
| "\n", | ||
| " dyn_type = CustomDynModel\n", | ||
| " fsw_type = fsw.SteeringImagerFSWModel" | ||
| " self.also_image_filter = []" |
There was a problem hiding this comment.
I don't see this "also_image_filter" is used anywhere.
| " observation, reward, terminated, truncated, info = env.step(action_dict)\n", | ||
| "\n", | ||
| " if all(terminated.values()) or all(truncated.values()):\n", | ||
| " if all(terminated.values()) or all(truncated.values()) or steps >= 5:\n", |
There was a problem hiding this comment.
I think it would be better to see the info message instead of just episode complete output. Is there any reason why you avoid it and made it to "warning" especially since you didn't actually run training in the script?
| "metadata": {}, | ||
| "source": [ | ||
| "Check [Training with RLlib PPO](../examples/rllib_training.ipynb) for an example on how to train the agent in this environment." | ||
| "A similar heuristic can be used for the single-picture case, where the expected reward is used instead of the rewards to compute the reward density per target with\n", |
There was a problem hiding this comment.
Just the order of the heuristic policy. I think the single image one should be first to be consistent.
| "\n", | ||
| "The observation space contains concatenated information regarding the spacecraft and individual targets, which naturally decomposes into two components: a spacecraft state vector and a set of target-specific state vectors. This representation motivates the use of shared target encoders, attention mechanisms, and permutation-invariant pooling operations instead of applying an MLP directly to the flattened observation. \n", | ||
| "\n", | ||
| "Then, custom actor and critic modules are defined for training with PPO." |
There was a problem hiding this comment.
Add recommended (or used) ray and torch version for this training.
Also, did you rewrite all of the code here from the Ray codebase? I assume some parts were copied from other sources and then combined or modified. It would be helpful to clearly show which parts came from the original source and which parts you changed.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Then, the training hyperparameters and modules parameters are defined, as well as the custom RLModule." |
There was a problem hiding this comment.
Add short comment that those parameters are chosen by hyperparameter search.
| ")\n", | ||
| "ppo_config.rl_module(**rl_module_args)\n", | ||
| "\n", | ||
| "# Uncomment to run training\n", |
There was a problem hiding this comment.
ray and tune are not imported which are used in the commented code.
Description
Closes #342
Updates the cloud with reimaging example script to incorporate recent changes and new actor-critic network architecture.
Type of change
How should this pull request be reviewed?
Future Work
Update with the related publication information once available.
Checklist